WIP: Cfs tests #1913
WIP: Cfs tests #1913
Conversation
Johan-Liebert1
force-pushed
the
cfs-tests
branch
from
4d359af to
1229ffa
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds tests for composefs soft reboot functionality, introducing a reset-soft-reboot command and updating the CLI to handle different soft reboot modes. The changes are generally well-implemented, but I've identified several leftover debugging statements that should be removed to clean up the output. Additionally, there's a critical typo in a test script that needs to be fixed, and a minor encapsulation concern with a newly public field.
tmt/tests/booted/test-soft-reboot.nu
Outdated
| # See ../bug-soft-reboot.md - TMT cannot handle systemd soft-reboots | ||
| ostree admin prepare-soft-reboot --reset | ||
| } else { | ||
| bootc internals perp-soft-reboot --reset |
There was a problem hiding this comment.
There is a typo in the command: perp-soft-reboot should be prep-soft-reboot. This will cause the test to fail.
bootc internals prep-soft-reboot --reset
| prepare_soft_reboot_composefs(&storage, &booted_cfs, &deployment, reboot) | ||
| .await | ||
| if reset { | ||
| return reset_soft_reboot(); |
There was a problem hiding this comment.
The function reset_soft_reboot returns a Result. It's good practice to handle the result here, even if it's just to propagate the error. The current implementation will ignore any potential errors from reset_soft_reboot.
reset_soft_reboot()?| # Assume the localhost/bootc image is up to date, and just run tests. | ||
| # Useful for iterating on tests quickly. | ||
| test-tmt-nobuild *ARGS: | ||
| echo {{ARGS}} |
There was a problem hiding this comment.
This echo statement appears to be for debugging purposes. It should be removed to keep the build output clean.
|
|
||
| let Some(nextroot) = nextroot else { | ||
| tracing::debug!("Nextroot is not a directory"); | ||
| println!("No deployment staged for soft rebooting"); |
There was a problem hiding this comment.
The messages printed to the console for user feedback should ideally be directed to stderr instead of stdout. This is a common practice for diagnostic messages, allowing users to separate program output from informational messages. For example, eprintln!("No deployment staged for soft rebooting");.
| println!("No deployment staged for soft rebooting"); | |
| eprintln!("No deployment staged for soft rebooting"); |
|
|
||
| if !nextroot_mounted { | ||
| tracing::debug!("Nextroot is not a mountpoint"); | ||
| println!("No deployment staged for soft rebooting"); |
There was a problem hiding this comment.
Similar to the previous comment, this user-facing message should be printed to stderr rather than stdout.
| println!("No deployment staged for soft rebooting"); | |
| eprintln!("No deployment staged for soft rebooting"); |
| /// This spawns a separate VM per test plan to avoid state leakage between tests. | ||
| #[context("Running TMT tests")] | ||
| pub(crate) fn run_tmt(sh: &Shell, args: &RunTmtArgs) -> Result<()> { | ||
| println!("RunTmtArgs: {args:#?}"); |
There was a problem hiding this comment.
This println! appears to be a debugging statement. Please remove it to avoid cluttering the test output.
|
|
||
| let firmware_args = build_firmware_args(sh, image)?; | ||
|
|
||
| println!("firmware_args: {firmware_args:#?}"); |
There was a problem hiding this comment.
This println! seems to be a debugging statement. Please remove it to avoid cluttering the test output.
|
|
||
| let imgsrc = imgsrc | ||
|
|
||
| print $"USING IMAGE: ($imgsrc)" |
There was a problem hiding this comment.
This print statement appears to be for debugging. It should be removed to keep the test output clean.
| if ($imgsrc | str ends-with "-local") { | ||
| bootc image copy-to-storage | ||
|
|
||
| print $"After copy-to-storage" |
There was a problem hiding this comment.
This print statement appears to be for debugging. It should be removed to keep the test output clean.
| # Now, switch into the new image | ||
| print $"Applying ($imgsrc)" | ||
| bootc switch --transport containers-storage ($imgsrc) | ||
| print $"Switch to ($imgsrc) COMPLETE" |
There was a problem hiding this comment.
This print statement appears to be for debugging. It should be removed to keep the test output clean.
Johan-Liebert1
force-pushed
the
cfs-tests
branch
from
1229ffa to
2462400
Compare
Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Aligning with ostree API, now we only initiate soft-reboot if `--apply` is passed to `bootc update`, `bootc switch`, else we only prepare the soft reboot Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
After bootc/commit/49d753f996747a9b1f531abf35ba4e207cf4f020, composefs-rs saves config in the format `oci-config-sha256:`. Update to match the same Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Johan-Liebert1
force-pushed
the
cfs-tests
branch
from
e166674 to
353bee2
Compare
When `--download-only` is passed, only download the image into the composefs repository but don't finalize it. Conver the /run/composefs/staged-deployment to a JSON file and Add a finalization_locked field depending upon which the finalize service will either finalize the staged deployment or leave it as is for garbage collection (even though GC is not fully implemented right now). Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
1 participant
No description provided.